Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove VotesERC20 from AzoriusTxBuilder #1645

Merged
merged 4 commits into from
May 7, 2024

Conversation

adamgall
Copy link
Member

@adamgall adamgall commented May 5, 2024

Please review and merge #1644 first.

@adamgall adamgall self-assigned this May 5, 2024
Copy link

netlify bot commented May 5, 2024

Deploy Preview for fractal-dev ready!

Name Link
🔨 Latest commit b72f908
🔍 Latest deploy log https://app.netlify.com/sites/fractal-dev/deploys/663ab558755fa300084ef15d
😎 Deploy Preview https://deploy-preview-1645.app.dev.fractalframework.xyz
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@DarksightKellar DarksightKellar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question but otherwise lgtm!

contract: Contract,
method: string,
params: any[],
const finishBuildingConractCall = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: finishBuildingConractCall -> finishBuildingContractCall

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gah. i almost just want to leave it as is for now, because fixing this will cause me to need to rebase all of the branches above this one 😅

i plan on consolidating this back into one function anyway, once the the ethers-style function can be removed. I'll deal with it then.

@@ -184,6 +184,45 @@ export const buildContractCall = (
);
};

export const buildContractCall = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to go away at some point? Noticing the -viem variant below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure is!

@adamgall adamgall force-pushed the remove-votestokenmastercopycontract-from-usesafecontracts branch from f0c3317 to 0829528 Compare May 6, 2024 18:58
@adamgall adamgall force-pushed the remove-voteserc20-from-azoriustxbuilder branch from e5fab96 to a919284 Compare May 6, 2024 19:00
@adamgall adamgall force-pushed the remove-votestokenmastercopycontract-from-usesafecontracts branch from 0829528 to a2f5908 Compare May 6, 2024 19:22
@adamgall adamgall force-pushed the remove-voteserc20-from-azoriustxbuilder branch from a919284 to ede0aa4 Compare May 6, 2024 19:23
@adamgall adamgall force-pushed the remove-voteserc20-from-azoriustxbuilder branch from ede0aa4 to 1f0e490 Compare May 6, 2024 19:28
@adamgall adamgall force-pushed the remove-votestokenmastercopycontract-from-usesafecontracts branch from a2f5908 to f907412 Compare May 6, 2024 19:29
Comment on lines +558 to +560
if (!this.predictedTokenAddress) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this defined when existing token is used for DAO Creation? Ie set to the existing token's address

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH not sure. I assumed (perhaps incorrectly?) that this check here was safe, because if you look at the old code on line 561, we were forcing the value to be valid (this.predictedTokenAddress!).

I could just put that ! back in on new line 574 and remove this conditional early exit, but I don't like ! in typescript.

I suppose I should go and manually test this out, huh

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case when a user brings their own token to new Azorius DAO creation, this.predictedTokenAddress gets set to that existing token address. So, this function executes as expected. Thanks for the call out here.

Base automatically changed from remove-votestokenmastercopycontract-from-usesafecontracts to develop May 7, 2024 12:35
@adamgall adamgall marked this pull request as ready for review May 7, 2024 12:36
@adamgall adamgall merged commit a50f6d7 into develop May 7, 2024
7 checks passed
@adamgall adamgall deleted the remove-voteserc20-from-azoriustxbuilder branch May 7, 2024 23:17
adamgall added a commit that referenced this pull request May 8, 2024
…stxbuilder

Remove VotesERC20 from AzoriusTxBuilder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants